Skip to content

Fix/Perf/Refactor: buildableUnits and many related code#3220

Draft
VariableVince wants to merge 28 commits intomainfrom
some-leftovers
Draft

Fix/Perf/Refactor: buildableUnits and many related code#3220
VariableVince wants to merge 28 commits intomainfrom
some-leftovers

Conversation

@VariableVince
Copy link
Contributor

@VariableVince VariableVince commented Feb 16, 2026

Description:

After #3213 got merged, the change with largest impact in #3193 was done in such a different way that a new PR was needed

The idea in 3193 was to not always ask for Transport Ship from buildableUnits. In such a way that very little extra data was send to the worker. This had the biggest impact on performance (the idea was months older btw, see #2295). Now, we do it the other way around, by telling buildableUnits all unit types we want. Or we want them all (undefined). The downside is more data is send in the worker message. The upside is we have more options and can add more in this PR.

This PR implements some of the leftovers in 3193 on top of 3213 and adds more.

  • GameRunner: option to ask for no buildable units (null) in playerActions.

  • Game / PlayerImpl: Typesafety and some added perf: new type PlayerBuildableUnitType so callers of buidableUnits can never ask for the wrong type like e.g. UnitType.Train because it doesn't return data for that type. This type is now used in aother applicable places too for the same reason.

  • GameRunner: Fixes wrong assumption in Add units filter on playeractions for performance #3213: that only if units was undefined, we have to know canAttack. ClientGameRunner wants to know both, in case of a click on non-bordering land, to decide if it should auto-boat using a Transport Ship. So units is not undefined (we only ask for Transport Ship now which has a positive effect on performance for each click/tap) but we need canAttack still.
    Solved by removing the unit === undefined check before canAttack in playerActions. playerActions now has 3 modes: get all actions and all buildings (units undefined), get all actions and no buildings (units null), or get all actions and specific building (units contains Unit Types).

  • GameRunner: But with above solved, there was still no option to only get buildable units. While StructureIconsLayer, NukeTrajectoryPreviewLayer, BuildMenu and UnitsDisplay need only that. To not make playerActions more convoluted with more params or so, i've added a new function playerBuildables to only get buildable units. playerBuildables has 2 modes: get all buildings (units undefined) or get specific buildings (units contains Unit Types).

  • Have all callers of GameView .actions or .buildables, ultimately GameRunner playerActions and playerBuildables, ask for their type of units. Not only StructureIconsLayer and NukeTrajectoryPreviewLayer but also ClientGameRunner, BuildMenu, UnitsDisplay and PlayerPanel. Only MainRadialMenu needs all player buildable unit types including Transport Ship, so it can stay undefined there.

  • PlayerImpl: validStructureSpawnTiles did a filter on unit types to get isTerroritoryBound units, on every call again. It read this from unit info in DefaultConfig so while that's good for maintainability, other code already uses hardcoded StructureTypes and isisStructureType from Game.ts. Which has the same purpose and thus contains the same unit types. StructureTypes and isisStructureType do need manual maintainance outside of DefaultConfig. And are more bug prone/less type safe. Still, using them gives more speed compared to putting it in some get function in GameImpl for example (tested with buildableUnits and MIRVPerf.ts. So went with StructureTypes in validStructureSpawnTiles.

  • PlayerExecution: now validStructureSpawnTiles no longer needs isTerritoryBound (see the point above), PlayerExecution is the last place where it was used. Replaced it for isStructureType here too (since it has the same meaning and outcome).

  • Game.ts and DefaultConfig unitInfo: removed isTerritoryBound as it was only used in validStructureSpawnTiles and PlayerExecution and has been replaced in both (see the two points above).

  • PlayerImpl: Early return for non-upgradable unit types in findUnitToUpgrade. If it is upgradable, it will again check in canUpgradeUnit if the unit type is upgradable alas. Since canUpgradeUnit is also used by other code, this double check cannot be removed in a simple way. Still the early return makes the code faster as there are a number of units that can't be upgraded so we can skip all that happens in findUnitToUpgrade.

  • PlayerImpl: buildableUnits would do Object.values(UnitTypes) on every call. Now only get player buildable units, exclude MIRVWarhead, TradeShip, Train, SamMissile and Shell. Since a player doesn't build those by themselves, they are only build by Executions which use canBuild directly instead of buildableUnits. Added PlayerBuildableTypes to Game.ts in the same fashion as existing StructureTypes and isStructureType.

  • PlayerImpl: buildableUnits would check twice for tile!==null to decide to call findUnitToUpgrade and canBuild. Now once.

  • PlayerImpl: buildableUnits would call canBuild which checks Cost, but then it fetches the Cost itself too. Now just do this once with canBuild accepting it as optional param. This does not affect other code that calls canBuild.

  • StructureIconsLayer: In order to make type safety work in StructureIconsLayer for GhostUnit.buildableUnit.type too, changed type of interface BuildableUnit to PlayerBuildableType. Which is only more accurate. Same for uiState.ghostStructure and with that, renderUnitItem in UnitDisplay and setGhostStructure in InputHandler. All Structures are of PlayerBuildableType (there are even some in PlayerBuildableType that aren't Structures, but it is much more confined than UnitType).

  • RadialMenuElements: replace non-central ATTACK_UNIT_TYPES in RadialMenuElements with centralized BuildableAttackTypes too. Use PlayerBuildableUnitType for more type safety (can't by mistake add UnitType.Train to its build menu). Make use of StructureTypes and BuildableAttackTypes instead of adding items hardcoded line by line in getAllEnabledUnits.

Some related (perf) changes:

  • NationStructureBehavior: removed canUpgradeUnit check from maybeUpgradeStructure. Since it already checked this right before in findBestStructureToUpgrade, so only upgradable units are returned. Furthermore, UpgradeStructureExecution is called right after which also again checks canUpgradeUnit. So we're going from 3 times to 2 times canUpgradeUnit, small perf win on its own.

  • NationStructureBehavior: instead of hardcoded ruling out Defense Post for upgrade check in maybeSpawnStructure, check dynamically if type is upgradable. That way if defense posts ever do become upgradable, we don't run into a bug right away.

  • NationsStructureBehavior: in getTotalStructureDensity, we can pass an array of types as argument so we only have to get an array length once. It needs to ignore levels so we can't make use of other pre-defined functions in PlayerImpl (which were created to avoid array length calls), but at least this saves a few.

  • GameImpl/GameView nearbyUnits: it accepted "UnitType | UnitType[]" for tiles, but called UnitGrid nearbyUnits which mandates "UnitType | readonly UnitType[]". Made the requirement the same for GameImpl/GameView nearbyUnits. This way, we don't have make a shallow copy of the StructureTypes array everytime we want to send it as an argument, from validStructureSpawnTiles and Util.ts. Other callers aren't affected.

(#3235) - ClientGameRunner: removed two redundant myPlayer===null checks since that was already done right above.

(#3235) - BuildMenu: Make playerBuildables have default value of null (null was already set as one of the possible values). Now we can remove the "?? []" check in canBuildOrUpgrade, since we already checked if playerBuildables is null when we get there, and even skip the assignment to const buildableUnits and loop over playerActions.buildableUnits directly.

(#3235) - RadialMenuElements: don't call canBuildOrUpgrade 3x in CreateMenuElements for the .map on flattenedBuildTable, instead do it once and re-use outcome.

(#3234) - Game.ts and DefaultConfig unitInfo: removed canBuildTrainStation and expirimental properties, as they already weren't used anywhere anymore.

(#3234) - PlayerActionHandler: remove unused getPlayerActions, the only potential caller MainRadialMenu already just calls myPlayer.actions via GameView directly.

(#3234) - StructureIconsLayer: remove already unused PlayerActions

(#3233) - Worker.worker: correct some existing error messages

(#3233) - BuildMenu: Fix some comments.

  • And more

Please complete the following:

  • I have added screenshots for all UI updates
  • I process any text displayed to the user through translateText() and I've added it to the en.json file
  • I have added relevant tests to the test directory
  • I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced

Please put your Discord username so you can be contacted if a bug or regression is found:

tryout33

…yerBuildableTypes which includes Transport Ship. Accept only readonly which includes existing enum UnitType. buildableUnits only returns PlayerBuildableTypes that the player .. can build while Executions keep using canBuild directly
…leUnits can never ask for eg UnitType.Train when it doesn't return data for that type.

In order to make type safety work in StructureIconsLayer for GhostUnit.buildableUnit.type too, changed type of interface BuildableUnit to PlayerBuildableType too. Which is only more accurate. Same for uiState.ghostStructure and with that, renderUnitItem in UnitDisplay and setGhostStructure in InputHandler.

Have all callers ask for their type of units, not only StructureIconsLayer and NukeTrajectoryPreviewLayer which already did, but also ClientGameRunner, BuildMenu and PlayerPanel. Only MainRadialMenu needs all player buildable unit types including transport ship, so it can stay undefined there.

Added a 'null' argument so that PlayerPanel and one function in ClientGameRunner can ask for no buildableUnits at all, which will stop GameRunner from calling PlayerImpl buildableUnits. Just like GameRunner already doesn't call canAttack when tile is null.
@VariableVince VariableVince added this to the v30 milestone Feb 16, 2026
@VariableVince VariableVince self-assigned this Feb 16, 2026
@VariableVince VariableVince added the Performance Performance optimization label Feb 16, 2026
@VariableVince VariableVince requested a review from a team as a code owner February 16, 2026 01:25
@VariableVince VariableVince added Refactor Code cleanup, technical debt, refactoring, and architecture improvements. Bugfix Fixes a bug labels Feb 16, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 16, 2026

Walkthrough

Adds BuildableUnit and PlayerBuildableUnitType + BuildMenuTypes; changes player.actions to accept readonly PlayerBuildableUnitType[] | null; adds playerBuildables RPC and GameRunner.playerBuildables; removes several UnitInfo flags; replaces territoryBound checks with isStructureType/StructureTypes; updates many client call sites and UI types to use buildables.

Changes

Cohort / File(s) Summary
Core public API & types
src/core/game/Game.ts, src/core/game/GameView.ts, src/core/game/GameImpl.ts
Introduce BuildableUnit, PlayerBuildableUnitType, BuildMenuTypes, PlayerBuildableTypes, StructureTypes, helpers (isBuildableAttackType/isPlayerBuildableType); remove territoryBound/canBuildTrainStation/experimental; tighten nearbyUnits readonly typing.
GameRunner & worker RPCs
src/core/GameRunner.ts, src/core/worker/Worker.worker.ts, src/core/worker/WorkerClient.ts, src/core/worker/WorkerMessages.ts
Add playerBuildables API and worker message flow (player_buildables/player_buildables_result); change worker/client messages and method signatures to use `readonly PlayerBuildableUnitType[]
Player impl & execution logic
src/core/game/PlayerImpl.ts, src/core/execution/PlayerExecution.ts, src/core/execution/nation/NationStructureBehavior.ts, src/core/execution/Util.ts
buildableUnits returns BuildableUnit[] and accepts PlayerBuildableUnitType[]; replace territoryBound checks with isStructureType; use StructureTypes for nearby queries; simplify upgrade gating and density calculation.
Client runner & call sites
src/client/ClientGameRunner.ts, src/client/graphics/layers/NukeTrajectoryPreviewLayer.ts, src/client/graphics/layers/MainRadialMenu.ts, src/client/graphics/layers/PlayerPanel.ts
Call sites updated to use actions(tile, units?), actions(tile, null), or buildables(tile, units?); pass BuildMenuTypes/StructureTypes or explicit lists; added some non-null assertions for myPlayer.
Client UI types & build menus
src/client/InputHandler.ts, src/client/graphics/UIState.ts, src/client/graphics/layers/BuildMenu.ts, src/client/graphics/layers/UnitDisplay.ts, src/client/graphics/layers/StructureDrawingUtils.ts, src/client/graphics/layers/StructureIconsLayer.ts
Switch build-related types from UnitType to PlayerBuildableUnitType; replace PlayerActions usage with BuildableUnit[]; fetch buildables via BuildMenuTypes/buildables; rename members (e.g., playerActionsplayerBuildables).
Client menus & element logic
src/client/graphics/layers/RadialMenuElements.ts, src/client/graphics/layers/PlayerActionHandler.ts, src/client/graphics/layers/MainRadialMenu.ts
Generate enabled unit sets as PlayerBuildableUnitType; remove getPlayerActions; wire menus to playerBuildables; update filtering to use isBuildableAttackType.
Configuration
src/core/configuration/DefaultConfig.ts
Remove territoryBound and related flags from UnitInfo entries; simplify UnitInfo shape.
Misc signatures & imports
multiple files (src/core/worker/WorkerMessages.ts, src/core/worker/WorkerClient.ts, src/core/game/GameView.ts, etc.)
Adjust imports and method signatures to prefer PlayerBuildableUnitType, readonly unit arrays, nullable unit lists, and BuildableUnit return types; update many call sites to pass explicit unit filters or null.

Sequence Diagram(s)

sequenceDiagram
  participant UI as Client UI
  participant CGR as ClientGameRunner
  participant WC as WorkerClient
  participant WK as Worker
  participant GR as GameRunner
  participant P as PlayerImpl

  UI->>CGR: request buildables(tile, units?)
  CGR->>WC: worker.playerBuildables(playerID, x, y, units)
  WC->>WK: post "player_buildables"
  WK->>GR: gameRunner.playerBuildables(playerID, x, y, units)
  GR->>P: player.buildableUnits(tile, units?)
  P-->>GR: BuildableUnit[] (type, cost, canBuild/canUpgrade)
  GR-->>WK: player_buildables_result
  WK-->>WC: reply
  WC-->>CGR: resolve BuildableUnit[]
  CGR-->>UI: provide buildables to UI
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • evanpelle

Poem

🌱 Types rearranged, menus learn new names,
Ghosts wear buildable crowns in calmer games.
Readonly lists keep choices small and true,
Structures counted by a clearer view,
Build menu hums — a tidy world renewed.

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: refactoring and optimizing buildableUnits and related code, which is the core focus of the PR.
Merge Conflict Detection ✅ Passed ✅ No merge conflicts detected when merging into main
Description check ✅ Passed The PR description comprehensively explains the rationale, implementation details, and performance optimizations across multiple files.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot]

This comment was marked as resolved.

@github-project-automation github-project-automation bot moved this from Triage to Development in OpenFront Release Management Feb 16, 2026
coderabbitai[bot]

This comment was marked as resolved.

coderabbitai[bot]

This comment was marked as outdated.

coderabbitai[bot]
coderabbitai bot previously approved these changes Feb 16, 2026
coderabbitai[bot]

This comment was marked as off-topic.

coderabbitai[bot]

This comment was marked as resolved.

coderabbitai[bot]
coderabbitai bot previously approved these changes Feb 16, 2026
coderabbitai[bot]
coderabbitai bot previously approved these changes Feb 16, 2026
coderabbitai[bot]

This comment was marked as resolved.

coderabbitai[bot]
coderabbitai bot previously approved these changes Feb 17, 2026
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/core/game/PlayerImpl.ts (1)

983-991: Cost is still recomputed inside canUpgradeUnit (line 953).

You precompute cost at line 983 and pass it into canBuild — nice. But findUnitToUpgrade calls canUpgradeUnit, which independently recomputes this.mg.config().unitInfo(unit.type()).cost(this.mg, this) at line 953 to check gold. That is the same value as cost here.

Consider threading knownCost through findUnitToUpgradecanUpgradeUnit the same way you did for canBuild, so you compute cost exactly once per unit type.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/core/game/PlayerImpl.ts` around lines 983 - 991, Precompute cost once and
thread it into the upgrade checks: change findUnitToUpgrade to accept a
knownCost (or pass the already computed cost variable) and forward that into
canUpgradeUnit, then modify canUpgradeUnit to accept an optional knownCost
parameter and use it instead of calling this.mg.config().unitInfo(...).cost(...)
internally; update all call sites of findUnitToUpgrade and canUpgradeUnit to
pass the cost where available and fall back to computing cost only when
knownCost is not provided to preserve existing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/core/game/PlayerImpl.ts`:
- Around line 983-991: Precompute cost once and thread it into the upgrade
checks: change findUnitToUpgrade to accept a knownCost (or pass the already
computed cost variable) and forward that into canUpgradeUnit, then modify
canUpgradeUnit to accept an optional knownCost parameter and use it instead of
calling this.mg.config().unitInfo(...).cost(...) internally; update all call
sites of findUnitToUpgrade and canUpgradeUnit to pass the cost where available
and fall back to computing cost only when knownCost is not provided to preserve
existing behavior.

@evanpelle
Copy link
Collaborator

I'm having trouble understanding this PR since it contains unrelated fixes, would it be possible to break this up into multiple PRs?

@VariableVince
Copy link
Contributor Author

I'm having trouble understanding this PR since it contains unrelated fixes, would it be possible to break this up into multiple PRs?

Not that easy with my lacking git skills. Without those it will cost a lot of time to split up (or fix mistakes when i do try git magic and mess up) so may either take awhile or not happen alas

@VariableVince VariableVince marked this pull request as draft February 18, 2026 18:59
github-merge-queue bot pushed a commit that referenced this pull request Feb 18, 2026
…3233)

## Description:

PR 1/x in effort to break up PR
#3220.
Please see if these can be merged for v30.

- **BuildMenu**: remove one redundant comment about replacing an icon
(which has been done long ago already). And fix typo in one other
comment.
- **Worker.worker**: correct some existing error messages.

## Please complete the following:

- [x] I have added screenshots for all UI updates
- [x] I process any text displayed to the user through translateText()
and I've added it to the en.json file
- [x] I have added relevant tests to the test directory
- [x] I confirm I have thoroughly tested these changes and take full
responsibility for any bugs introduced

## Please put your Discord username so you can be contacted if a bug or
regression is found:

tryout33
DevelopingTom pushed a commit to DevelopingTom/OpenFrontIO that referenced this pull request Feb 18, 2026
## Description:

PR 2/x in effort to break up PR
openfrontio#3220.

Removes unused code and properties.

- **Game.ts** and **DefaultConfig** unitInfo: removed
_canBuildTrainStation_ and _expirimental_ properties, as they weren't
used anywhere anymore.

- **PlayerActionHandler**: remove unused getPlayerActions, the only
potential caller MainRadialMenu already just calls myPlayer.actions via
GameView directly.

- **StructureIconsLayer**: remove unused PlayerActions

## Please complete the following:

- [x] I have added screenshots for all UI updates
- [x] I process any text displayed to the user through translateText()
and I've added it to the en.json file
- [x] I have added relevant tests to the test directory
- [x] I confirm I have thoroughly tested these changes and take full
responsibility for any bugs introduced

## Please put your Discord username so you can be contacted if a bug or
regression is found:

tryout33
github-merge-queue bot pushed a commit that referenced this pull request Feb 18, 2026
## Description:

PR 3/x in effort to break up PR
#3220. Follows on already
merged #3233 and
#3234.

Please see if these can be merged for v30.

- **ClientGameRunner**: removed two redundant myPlayer===null checks
since that was already done right above, instead use !.
- **BuildMenu**: just like in UnitDisplay, assign public PlayerActions
default value of null. So that in canCreateOrBuild, where we already do
a === null check on it btw, we can safely skip the assignment to const
buildableUnits and just directly loop over
this.playerActions.buildableUnits.
- **RadialMenuElements**: don't call canBuildOrUpgrade 3x in
CreateMenuElements for the .map on flattenedBuildTable, instead do it
once and re-use outcome.

## Please complete the following:

- [x] I have added screenshots for all UI updates
- [x] I process any text displayed to the user through translateText()
and I've added it to the en.json file
- [x] I have added relevant tests to the test directory
- [x] I confirm I have thoroughly tested these changes and take full
responsibility for any bugs introduced

## Please put your Discord username so you can be contacted if a bug or
regression is found:

tryout33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bugfix Fixes a bug Performance Performance optimization Refactor Code cleanup, technical debt, refactoring, and architecture improvements.

Projects

Status: Development

Development

Successfully merging this pull request may close these issues.

2 participants

Comments